-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gRPC and test fixes #1712
gRPC and test fixes #1712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from the race which is the same/very similar to #1657 ... and might actually be a bug in the way RunContext is used ... This might need more investigation, but probably after the release.
5abc465
to
18c17d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just minor comments. And CI currently fails on TestComplicatedFileImportsForGRPC
.
checkModule := &CheckModule{t: t} | ||
modules.Register("k6/check", checkModule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this gives us a simple way to bridge JS with Go test code. This approach might help with some flaky tests that currently rely on timing instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just have to remember to use different packages for registering new k6 modules that help with testing... Given the singleton nature of the module registry, we don't want "pollution"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that could be a problem. Should we add some kind of Deregister
mechanism so that each test can clean up after itself? Since it's in internal
we don't have to make it public and could only use it in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it shouldn't really matter if such tests are in their own package, but sure, an internal de-registering function can be added in the future
18c17d7
to
24bf54c
Compare
Codecov Report
@@ Coverage Diff @@
## master #1712 +/- ##
==========================================
+ Coverage 71.40% 71.46% +0.06%
==========================================
Files 176 178 +2
Lines 13679 13719 +40
==========================================
+ Hits 9767 9804 +37
- Misses 3301 3303 +2
- Partials 611 612 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This adds a bunch of tests and changes one details in the gRPC JS API: instead of
grpc.newClient()
, we now have the more JS idiomaticnew grpc.Client()
.cc @rogchap